fix: preserve Dockerfile-specific .dockerignore files#1206
fix: preserve Dockerfile-specific .dockerignore files#1206lawrence3699 wants to merge 1 commit intodevcontainers:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes devcontainer builds so Dockerfile-specific ignore files (e.g. dev.Dockerfile.dockerignore) continue to apply when the CLI rewrites the Dockerfile into a temp folder (e.g. Dockerfile-with-features), aligning behavior with Docker’s documented .dockerignore lookup rules.
Changes:
- Copy
<source Dockerfile>.dockerignorealongside the generatedDockerfile-with-featuresfor direct Dockerfile builds. - Apply the same
.dockerignorepreservation for compose-backed builds when generating the compose build override Dockerfile. - Add a regression test covering a compose build that uses a non-standard Dockerfile name.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/spec-node/dockerignoreUtils.ts | Adds helper to copy Dockerfile-specific .dockerignore to the generated Dockerfile location. |
| src/spec-node/singleContainer.ts | Calls the helper when generating Dockerfile-with-features for direct Dockerfile builds. |
| src/spec-node/dockerCompose.ts | Calls the helper for compose builds after writing Dockerfile-with-features. |
| src/test/dockerignore.test.ts | Adds regression coverage ensuring the generated compose Dockerfile has the sibling .dockerignore. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const fakeDocker = path.join(root, 'fake-docker'); | ||
| await fs.writeFile(fakeDocker, `#!/bin/sh | ||
| set -eu |
There was a problem hiding this comment.
This test creates a fake docker executable as a #!/bin/sh script and relies on chmod + shebang execution. That approach will fail on Windows when running tests via Node’s child_process.spawn (even under Git Bash), because Windows cannot directly execute shebang scripts without an explicit interpreter. Consider generating a cross-platform fake (e.g. a small Node.js .js script invoked via node, or a .cmd wrapper on win32), or skipping this test on process.platform === 'win32'.
| const { context, dockerfilePath, target } = serviceInfo.build; | ||
| const resolvedDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : path.resolve(context, dockerfilePath); | ||
| const originalDockerfile = (await cliHost.readFile(resolvedDockerfilePath)).toString(); | ||
| sourceDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : path.resolve(context, dockerfilePath); |
There was a problem hiding this comment.
sourceDockerfilePath is resolved using Node's path.resolve(...) even though the rest of the compose path handling uses cliHost.path (which can be posix/win32 depending on the host). This can produce incorrect paths when the CLI host path semantics differ from the local Node process (e.g. WSL/remote scenarios), and would prevent reading/copying the Dockerfile-specific .dockerignore. Use cliHost.path.resolve(context, dockerfilePath) (and avoid mixing path and cliHost.path) to keep resolution consistent.
| sourceDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : path.resolve(context, dockerfilePath); | |
| sourceDockerfilePath = cliHost.path.isAbsolute(dockerfilePath) ? dockerfilePath : cliHost.path.resolve(context, dockerfilePath); |
Fixes #969
Summary
.dockerignorefile next to the generatedDockerfile-with-featuresWhy this is correct
When the CLI rewrites a Dockerfile into a temp folder, Docker stops seeing the original
<Dockerfile>.dockerignorefile because it only looks for Dockerfile-specific ignore files next to the Dockerfile it is actually building. Copying that sibling file alongside the generated Dockerfile preserves Docker's existing lookup and precedence rules.Validation
ts-noderepro aroundbuildAndExtendDockerCompose: before the patch the generated temp Dockerfile had no sibling.dockerignore; after the patch it does, with the original contents preserved./node_modules/.bin/tsc -b./node_modules/.bin/eslint src/spec-node/dockerCompose.ts src/spec-node/singleContainer.ts src/spec-node/dockerignoreUtils.ts src/test/dockerignore.test.tsI could not run the Docker-backed CLI test suite in this environment because Docker is not installed here.